Catch setting role exception and output a warning instead of stop migration process #33

runwuf · 2022-04-01T00:22:56Z

Proposed changes in this pull request

Catch exception when setting role fails and output a warning instead of stop the migration process.
This is a fix for migration from AWS RDS/Aurora as it stops when setting rol_config for rdstopmgr role.

Type (put an `x` where ever applicable)

Checklist

Please put an x against the checkboxes. Write a small comment explaining if its N/A (not applicable)

All the tests are passing after the introduction of new changes.
Added tests respective to the part of code I have written.
Added proper documentation where ever applicable (in code and README.md).

Optional extra information

ghost

Sorry for the time to get this review. Some comments:

What is the exact error? It's unclear from both the PR text and the commit message; this is a statement that's being performed on the target db as far as I can understand; why should this be a problem when migrating from RDS/Aurora?
Catching "ProgrammingError" is too broad here IMHO and risks going on even when other issues arise; can't you find a more specific exception?

aiven_db_migrate/migrate/pgmigrate.py

…ration process

runwuf · 2022-04-26T23:06:13Z

@alanfranz-at-work @carobme I've added this new flag -fr , --filtered-roles to filter roles not to be migrated, please let me know your feedback then I'll add to the documentation.

aiven_db_migrate/migrate/pgmigrate.py

ghost

I think the "filtered" route is better, but will need some refinements.

please remember to dismiss stale reviews otherwise we may not notice your PR in the review queue!

aiven_db_migrate/migrate/pgmigrate.py

ghost · 2022-04-27T06:20:04Z

aiven_db_migrate/migrate/pgmigrate.py

+                        message="role created",
+                    )
+                # display warning when ProgrammingErrorERROR 42501: InsufficientPrivilege: permission denied to set parameter for a role
+                except psycopg2.errors.InsufficientPrivilege:


If you choose to go for the "filtered roles" route, why do you still need this change? Isn't this overlapping?

I hope to keep this change, with the filtered roles flag, user needs to know exactly what roles to filter before the migration and supply that to the cli argument, should there be any special setting with this role that is unable to be set with avnadmin's permission it would stop the migration as the exception is not caught. I think failing to set a role should not stop the migration process completely.

The problem is... it's quite a bold statement. We'd need to be sure that such role is not needed in the destination database. If we go on and then we discover after months that the migrated db doesn't work anymore, that's an issue. It's better to fail fast.

Again, this will impact everything: aiven_db_migrate is a library that could be used anywhere (and it's used in aiven-core as well, as a library), we're adding a very generic workaround for a very narrow use case.

This is an AWS-specific thing. Maybe an idea could be add a specific flag to the command line, something like --ignore-source-aws-roles, and you put AWS-specific roles there, and pass them to filtered roles.

that sounds like a good idea, I could just add rdstopmgr to the filtered_roles if --ignore-source-aws-roles is specified, do you think we should include all the rds* roles when the flag specified? and a similar flag such as --ignore-source-cloudsql-roles? one concern is if aws and google changes the names of these roles, probably not something we want to maintain in the migration tool code...

ghost

I don't think this can go in yet :-/

Still pending question:
I still don't understand why this happens and how, can you explain your setup and some details from the rdstopmgr role? Is it got some permission that we don't allow the end-user to assign? Could you paste some more detailed traceback?

ghost · 2022-04-28T08:13:11Z

aiven_db_migrate/migrate/pgmigrate.py

+        self,
+        conn_info: Union[str, Dict[str, Any]],
+        filtered_db: Optional[str] = None,
+        filtered_roles: Tuple[str] = ((), ),


The type for filtered_roles is wrong (tuple has fixed size, you need sequence or list or set) and the default argument type doesn't match.

ghost · 2022-04-28T08:14:02Z

aiven_db_migrate/migrate/pgmigrate.py

        if not self._pg_roles:
            # exclude system roles
-            roles = self.c("SELECT quote_ident(rolname) as safe_rolname, * FROM pg_catalog.pg_roles WHERE oid > 16384")
+            roles = self.c(f"SELECT quote_ident(rolname) as safe_rolname, * FROM pg_catalog.pg_roles WHERE oid > 16384")


remove f-string as it's not used anymore, please

ghost · 2022-04-28T08:16:25Z

aiven_db_migrate/migrate/pgmigrate.py

            for r in roles:
                rolname = r["rolname"]
+
+                if rolname in self.filtered_roles:
+                    self.log.debug(f'Skipping filtered role: [{r["rolname"]}]')


nit: loggers have their own formatting system, no need to use f-strings.

ghost · 2022-04-28T08:17:02Z

aiven_db_migrate/migrate/pgmigrate.py

@@ -738,15 +754,20 @@ def __init__(
        verbose: bool = False,
        mangle: bool = False,
        filtered_db: Optional[str] = None,
+        filtered_roles: Tuple[str] = ((), ),


Again, typing issues

ghost · 2022-04-28T08:17:45Z

aiven_db_migrate/migrate/pgmigrate.py

+                # display warning when ProgrammingErrorERROR 42501: InsufficientPrivilege: permission denied to set parameter for a role
+                except psycopg2.errors.InsufficientPrivilege:
+                    self.log.warning(
+                        f'Setting [{role.rolname}]: [{key}] = [{value}] failed.  psycopg2.errors.InsufficientPrivilege'


nit: logger formatting

ghost · 2022-04-28T08:21:00Z

aiven_db_migrate/migrate/pgmigrate.py

@@ -1343,6 +1372,9 @@ def main(args=None, *, prog="pg_migrate"):
    else:
        logging.basicConfig(level=logging.INFO, format=log_format)

+    if not args.filtered_roles:
+        args.filtered_roles = ()


the parser has already the support for default values. but this can't work. should be an empty string as default value, I think.

then, down there, you could do something like

filtered_roles=[ role for role in args.filtered_roles.split(",") if role != "" ] (untested code)

so that the filtered_roles sequence is actually empty when passing it to the main func.

runwuf · 2022-04-29T04:34:20Z

@alanfranz-at-work
Here's the details of the aws specific issue, when creating a aws rds postgresql or aurora postgresql, both create these users out of the box

 rds_superuser
 rds_replication
 rds_iam
 rds_password
 rds_ad
 rdstopmgr
 postgres
 rdsrepladmin

where this rdstopmgr got the following rolconfig

SELECT rolname,rolconfig FROM pg_catalog.pg_roles WHERE rolname='rdstopmgr';
  rolname  |                                                                                                                                           rolconfig
-----------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 rdstopmgr | {log_statement=all,log_min_error_statement=debug5,log_min_messages=panic,exit_on_error=0,statement_timeout=0,TimeZone=utc,"search_path=pg_catalog, public",auto_explain.log_min_duration=-1,role=rdstopmgr,temp_file_limit=-1,pg_hint_plan.enable_hint=off,default_transaction_read_only=off}

when the migration tool try to set log_statement=all for rdstopmgr role on aiven postgresql it throws psycopg2.errors.InsufficientPrivilege with avnadmin

This can be reproduced by creating a new aws rds or aurora and a new aiven postgresql and run the migration tool. First time the migration tool would stop as soon as it failed to set the log_statement=all for rdstopmgr role, running the migration tool again it will continue as the rdstopmgr role has been created already.

runwuf requested a review from a team as a code owner April 1, 2022 00:22

runwuf force-pushed the fwu-setting-role-warning branch from 747330d to 135af39 Compare April 1, 2022 00:30

ghost suggested changes Apr 13, 2022

View reviewed changes

carobme reviewed Apr 13, 2022

View reviewed changes

aiven_db_migrate/migrate/pgmigrate.py Show resolved Hide resolved

runwuf force-pushed the fwu-setting-role-warning branch 2 times, most recently from 0e09045 to 0e871c3 Compare April 26, 2022 18:46

catch setting role exception and output a warning instead of stop mig…

da827b4

…ration process

runwuf force-pushed the fwu-setting-role-warning branch from 0e871c3 to da827b4 Compare April 26, 2022 18:54

add -fr , --filtered-roles flag to filter roles not to be migrated

2759321

runwuf commented Apr 26, 2022

View reviewed changes

aiven_db_migrate/migrate/pgmigrate.py Outdated Show resolved Hide resolved

runwuf requested review from carobme and a user April 26, 2022 23:19

ghost suggested changes Apr 27, 2022

View reviewed changes

filter roles feature improvements

7a98a1c

runwuf force-pushed the fwu-setting-role-warning branch from 1a6cc73 to 7a98a1c Compare April 28, 2022 06:36

runwuf requested a review from a user April 28, 2022 06:38

ghost suggested changes Apr 28, 2022

View reviewed changes

carobme removed their request for review May 18, 2022 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Catch setting role exception and output a warning instead of stop migration process #33

Catch setting role exception and output a warning instead of stop migration process #33

runwuf commented Apr 1, 2022 •

edited

Loading

ghost left a comment

runwuf commented Apr 26, 2022

ghost left a comment

ghost Apr 27, 2022

runwuf Apr 28, 2022

ghost Apr 28, 2022 •

edited by ghost

Loading

runwuf Apr 29, 2022

ghost left a comment

ghost Apr 28, 2022

ghost Apr 28, 2022

ghost Apr 28, 2022

ghost Apr 28, 2022

ghost Apr 28, 2022

ghost Apr 28, 2022 •

edited by ghost

Loading

runwuf commented Apr 29, 2022

Catch setting role exception and output a warning instead of stop migration process #33

Are you sure you want to change the base?

Catch setting role exception and output a warning instead of stop migration process #33

Conversation

runwuf commented Apr 1, 2022 • edited Loading

Proposed changes in this pull request

Type (put an x where ever applicable)

Checklist

Optional extra information

ghost left a comment

Choose a reason for hiding this comment

runwuf commented Apr 26, 2022

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 28, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 28, 2022 • edited by ghost Loading

Choose a reason for hiding this comment

runwuf commented Apr 29, 2022

runwuf commented Apr 1, 2022 •

edited

Loading

Type (put an `x` where ever applicable)

ghost Apr 28, 2022 •

edited by ghost

Loading

ghost Apr 28, 2022 •

edited by ghost

Loading